revise the schema default type not null by congbobo184 · Pull Request #3752 · apache/pulsar

congbobo184 · 2019-03-05T02:44:57Z

Motivation

Modifications

Support define not not allow null field in schema

Verifying this change

Add not allow null field schema verify

Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (yes)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

jerrypeng · 2019-03-05T02:48:23Z

@congbobo184 while I am ok if we decide to remove allow null from set by default, my only concern is this ix NOT a backwards compatible change.

merlimat

This is breaking backward compatibility.

There needs to be a migration plan that allows us to identify that a schema was created with an earlier version of the client library and thus will let through a one-time no-check migration to the new default.

merlimat · 2019-03-05T03:05:27Z

Since the Python schema generation is following the current behavior of the Java client. We also need to change the defaults there. Again, with migration path in mind.

sijie · 2019-03-05T03:22:30Z

@congbobo184 @jerrypeng @merlimat :

I don't think we should attempt to involve any data migration between schemas, that will be a nightmare.

what I would suggest here:

adding a flag allowNull (by default to be true for backward compatibility). users have ability to disable allowNull.
if user disabled allowNull, the AvroSchema will add a property entry avro.allowNull = false to the properties map of schema info. This property will be used for telling how the avro schema is generated. avro.allowNull doesn't exist or exists as true means the schema is generated using AllowNull; avro.allowNull == false means the schema is generated NOT using AllowNull.

so there are nothing else to be changed. but the documentation should be updated to highlight allowNull will introduce incompatibilities when same POJO classes used in other AVRO system.

merlimat · 2019-03-05T05:16:10Z

Adding the flag and leave allowNull=true by default is good and easy.

I think though that using allowNull=false would be a better default option, but it doesn't have to be done right now in this PR.

Regarding the migration path, I'm not referring to migrate the schema definition, but rather to allow a one-time "incompatible" schema evolution marked with a version in pulsar schema registry.

sijie · 2019-03-05T05:21:12Z

Regarding the migration path, I'm not referring to migrate the schema definition, but rather to allow a one-time "incompatible" schema evolution marked with a version in pulsar schema registry.

supporting "incompatible" schema can't really be done using POJO schemas. we can support that using the generic schema stuffs that I recently work on. but there are still a lot of work to complete in that piece before we can claim we are ready to take such "incompatible" migration.

merlimat · 2019-03-05T05:36:07Z

In this case the problem is that, if we change the default setting, the schema extracted from a given POJO will change. The user defined will still be the same though, as the data serialized by it.

sijie · 2019-03-05T05:59:30Z

@merlimat

The user defined will still be the same though, as the data serialized by it.

the user defined will be still the same. that's fine.
but in a AllowNull schema, a record can be omitting fields; but in a NOT AllowNull schema, all fields have constraints. so if a record is serialized with AllowNull schema is not SAFE to be deserialized by a NOT AllowNull schema even the user defined POJO are same.

The other way around is working - you are okay to use a AllowNull schema to deserialize the data serialized by a NOT AllowNull schema.

With that being explained, if we are using POJO schema, we can't really just "migrate" the schema in schema registry (or it is very very tricky to do so). We can do it by using MultiVersionedGenericSchema which I introduced a few days ago. For each message, it will be using the right schema to deserialize the record into a GenericRecord. But that's probably not useful for applications are using POJO schemas.

jerrypeng · 2019-03-05T07:21:29Z

@sijie @congbobo184 @merlimat if we are removing AllowNull or allowing the flag to be configurable, should we also do it for JSONSchema:

https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/JSONSchema.java#L63

sijie · 2019-03-05T07:47:41Z

@jerrypeng I am not sure we need to do it for JSON. because there is no real schema in JSON data. every field in a json struct is optional, no?

merlimat · 2019-03-05T19:22:33Z

Thinking on this a bit more, I believe a better approach is to let the user specify the Avro schema definition directly, either in form of schema object or in json string.

That is because:

The schema definition is already the source of truth
I think that will be less confusing to people instead of thinking about whether to set allowNull=true/false and the implications of that.

The recommendation in the docs would be to pass the schema if you already have one.

sijie · 2019-03-06T02:07:02Z

@merlimat

let the user specify the Avro schema definition directly, either in form of schema object or in json string.

+1 on supporting specifying avro schema (via schema object or json). that's one of the schema tasks in my TODO list related schema builder.

Although I think it should be done in a separate PR.

codelipenghui · 2019-03-06T03:18:23Z

 * with the License.  You may obtain a copy of the License at
 *
- *   http://www.apache.org/licenses/LICENSE-2.0
+ * http://www.apache.org/licenses/LICENSE-2.0


format license header

codelipenghui · 2019-03-06T03:19:31Z

     */
    static <K, V> Schema<KeyValue<K, V>> KeyValue(Class<K> key, Class<V> value, SchemaType type) {
-        return DefaultImplementation.newKeyValueSchema(key, value, type);
+        return DefaultImplementation.newKeyValueSchema(key, value, type,true);


add blank space

codelipenghui · 2019-03-06T03:19:39Z

     */
    static <K, V> Schema<KeyValue<K, V>> KeyValue(Class<K> key, Class<V> value) {
-        return DefaultImplementation.newKeyValueSchema(key, value, SchemaType.JSON);
+        return DefaultImplementation.newKeyValueSchema(key, value, SchemaType.JSON,true);


add blank space

codelipenghui · 2019-03-06T03:19:50Z

    }

-    public static <T> Schema<T> newAvroSchema(Class<T> clazz) {
+    public static <T> Schema<T> newAvroSchema(Class<T> clazz,Boolean allowNull) {


add blank space

codelipenghui · 2019-03-06T03:19:58Z

-                () -> (Schema<T>) getStaticMethod("org.apache.pulsar.client.impl.schema.AvroSchema", "of", Class.class)
-                        .invoke(null, clazz));
+                () -> (Schema<T>) getStaticMethod("org.apache.pulsar.client.impl.schema.AvroSchema", "of", Class.class,Boolean.class)
+                        .invoke(null, clazz,allowNull));


add blank space

congbobo184 · 2019-03-16T08:34:58Z

run Integration Tests

congbobo184 · 2019-03-16T09:34:58Z

run Integration Tests

congbobo184 · 2019-03-16T13:59:06Z

run Integration Tests

congbobo184 · 2019-03-17T10:38:49Z

run java8 tests

congbobo184 · 2019-03-17T10:39:46Z

run Integration Tests

congbobo184 · 2019-03-17T12:58:47Z

run java8 tests

congbobo184 · 2019-03-17T12:59:04Z

run Integration Tests

congbobo184 · 2019-03-17T14:26:02Z

run Integration Tests

congbobo184 · 2019-03-18T02:12:05Z

run Integration Tests

congbobo184 · 2019-03-18T04:57:53Z

run Integration Tests

congbobo184 · 2019-03-18T07:14:48Z

run Integration Tests

congbobo184 · 2019-03-18T09:04:45Z

run Integration Tests

congbobo184 · 2019-03-19T10:28:48Z

run Integration Tests

congbobo184 · 2019-03-19T12:25:07Z

run Integration Tests

Fix #3741 Support define not not allow null field in schema Add not allow null field schema verify Does this pull request potentially affect one of the following parts: If yes was chosen, please highlight the changes Dependencies (does it add or upgrade a dependency): (no) The public API: (no) The schema: (yes) The default values of configurations: (no) The wire protocol: (no) The rest endpoints: (no) The admin cli options: (no) Anything that affects deployment: (no)

merlimat · 2019-04-01T17:46:47Z

Merged in 2.3.1 at
6e76af2

congbobo added 4 commits March 2, 2019 09:08

define default schema unable null

3749644

Merge remote-tracking branch 'apache/master' into schema_type_notnull

21cf4f4

Merge remote-tracking branch 'apache/master' into schema_type_notnull

ed46db4

Modify the Schema default unableNull test

8c06348

congbobo184 mentioned this pull request Mar 5, 2019

POJO AvroSchema always allowNull #3741

Closed

merlimat requested changes Mar 5, 2019

View reviewed changes

congbobo added 2 commits March 6, 2019 11:00

Modify the Schema type not null

0c68e9f

recover the format

8241d69

codelipenghui reviewed Mar 6, 2019

View reviewed changes

congbobo added 6 commits March 6, 2019 11:38

Modify the code format

bbda7b4

recover the code format

5bdc4da

function topic schema by default allow null need to fix

f24932e

Modify the code format

9040c82

Fix the test define schema type AllowNull

ed118b3

fix the test schema type allow null

3070234

congbobo added 5 commits March 15, 2019 18:01

add schema definition builder

5e37a97

Add genericity

4ef4be4

modify the test

ac4f047

modify the test

841dad5

modify the test

eab205f

congbobo added 2 commits March 17, 2019 01:30

Merge remote-tracking branch 'apache/master' into schema_type_notnull

c81f150

remove the dependency

7c59bac

remove the dependency

36a8977

Merge remote-tracking branch 'apache/master' into schema_type_notnull

2c9a5f9

sijie modified the milestones: 2.4.0, 2.3.1 Mar 19, 2019

sijie merged commit 1a1c557 into apache:master Mar 19, 2019

psilos mentioned this pull request Apr 18, 2020

Avro Schema Definition flags are not exposed on Pulsar Functions #6765

Closed

sijie mentioned this pull request Apr 18, 2020

ISSUE-6765: Avro Schema Definition flags are not exposed on Pulsar Functions streamnative/pulsar-archived#893

Closed

Uh oh!

Conversation

congbobo184 commented Mar 5, 2019 • edited by codelipenghui Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Uh oh!

jerrypeng commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

merlimat commented Mar 5, 2019

Uh oh!

sijie commented Mar 5, 2019

Uh oh!

merlimat commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sijie commented Mar 5, 2019

Uh oh!

merlimat commented Mar 5, 2019

Uh oh!

sijie commented Mar 5, 2019

Uh oh!

jerrypeng commented Mar 5, 2019

Uh oh!

sijie commented Mar 5, 2019

Uh oh!

merlimat commented Mar 5, 2019

Uh oh!

sijie commented Mar 6, 2019

Uh oh!

codelipenghui Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

codelipenghui Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

codelipenghui Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

codelipenghui Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

codelipenghui Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

congbobo184 commented Mar 16, 2019

Uh oh!

congbobo184 commented Mar 16, 2019

Uh oh!

congbobo184 commented Mar 16, 2019

Uh oh!

congbobo184 commented Mar 17, 2019

Uh oh!

congbobo184 commented Mar 17, 2019

Uh oh!

congbobo184 commented Mar 17, 2019

Uh oh!

congbobo184 commented Mar 17, 2019

Uh oh!

congbobo184 commented Mar 17, 2019

Uh oh!

congbobo184 commented Mar 18, 2019

Uh oh!

congbobo184 commented Mar 18, 2019

Uh oh!

congbobo184 commented Mar 18, 2019

Uh oh!

congbobo184 commented Mar 18, 2019

Uh oh!

congbobo184 commented Mar 19, 2019

Uh oh!

congbobo184 commented Mar 19, 2019

Uh oh!

merlimat commented Apr 1, 2019

Uh oh!

congbobo184 commented Mar 5, 2019 •

edited by codelipenghui

Loading

jerrypeng commented Mar 5, 2019 •

edited

Loading

merlimat commented Mar 5, 2019 •

edited

Loading